Repository navigation
fix(compliance): drop comply_test_controller from canonical_format_validate_input required_tools (OR-gate) - #7405
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, well-scoped compliance fix.
This PR removes comply_test_controller from required_tools on the canonical_format_validate_input universal compliance storyboard, correcting an OR-gate misselection (confirmed at sdk-runner-capability-gates.test.cjs:697) so agents are matched on validate_input rather than any controller-exposing agent. Controller seeding remains intact via the separate requires: [controller] load-time gate.
Checks performed:
- Changeset present and correctly scoped as
patch(.changeset/canonical-format-validate-input-required-tools.md); non-breaking wire change, so patch type is appropriate. - No
dist/**released artifact touched — immutability preserved. - Change is under
static/compliance/source/**, not a schema source file, so no schema↔docs coherence obligation triggered; no oneOf discriminator impact. - No test breakage; no governance-precedent conflict.
Non-blocking follow-up noted by reviewer: sibling 3.2 storyboards (ctv-experience-validate-input, premium-display-canonical-validation) may carry the same latent OR-gate shape — worth a future look but not required here.
No Critical/High/Medium findings. high_risk false, gated_paths false, no author-team gate. Falls through to row 9 → approve. (review_decision is REVIEW_REQUIRED, but with gated_paths false that does not force escalation.)
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated path requires human/CODEOWNERS review.
This PR modifies .github/workflows/training-agent-storyboards.yml, which falls under the repo's hard, path-based approval gate (gated_paths: true). The required review status is REVIEW_REQUIRED, not APPROVED, so the gate is not satisfied and the PR cannot be auto-approved despite a clean diff.
Blocking findings: none.
Medium findings: none.
Escalation reasons:
.github/workflows/training-agent-storyboards.yml(modified) matches the gated path glob.github/workflows/**andreview_decisionisREVIEW_REQUIRED. Human/CODEOWNERS approval is required before merge.
Review substance is clean: the change lowers the current-surface passing-step floors by exactly 1 for governance (161→160) and brand (116→115) in both the workflow YAML and run-storyboards-matrix.sh, kept in lockstep, as the correct minimal counterpart to removing comply_test_controller from canonical-format-validate-input's required_tools. A matching changeset is present. No Critical/High/Medium findings. The escalation is purely the deterministic gated-path gate, not a code concern.
Why human review
- .github/workflows/training-agent-storyboards.yml (modified) matches gated path .github/workflows/** and review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required (row 2)
- This PR touches a path under a hard, non-overridable approval gate (.github/workflows/training-agent-storyboards.yml (modified) matches
.github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
aao-secretariat
Bot
added
the
ladon/needs-human-review
|
@bokelley this one needs a human pass when you are in here. Ladon approved it on the first push and that approval was dismissed by the two follow-up commits, so there is no standing review and it sits on Five files, but only one of them is the fix: dropping The other four files are the consequence, and they are the part worth your judgment rather than a skim. So both I am flagging the de-ratchet rather than burying it: a lowered floor is exactly what that guard exists to make someone look at. If you would rather the floors stayed and the storyboard kept running for those tenants, that is a different fix and I will take the redline. #7406 is the 3.1.x half and is clean and approved. |
332df00 to
904aa37
Compare
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated path requires human/CODEOWNERS approval.
This PR modifies .github/workflows/training-agent-storyboards.yml, which matches the hard, path-based approval gate .github/workflows/** (gated_paths: true). The current GitHub review decision is REVIEW_REQUIRED, not APPROVED, so row 2 of the decision table fires: Ladon cannot auto-approve or plain-comment until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
The reviewer found no Critical/High/Medium findings — the change (dropping the universal comply_test_controller from required_tools to fix OR-gate over-selection, then lowering the min_passing_steps floors consistently in three pinned locations) is mechanically clean with a correct patch changeset. But the diff cleanliness does not lift a hard gate; only a recorded APPROVED review does.
Escalation reasons
.github/workflows/training-agent-storyboards.yml(modified) is under the hard approval gate.github/workflows/**andreview_decisionisREVIEW_REQUIRED, notAPPROVED— human/CODEOWNERS approval required (row 2).
Why human review
- .github/workflows/training-agent-storyboards.yml (modified) matches gated path .github/workflows/** and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required (row 2)
- This PR touches a path under a hard, non-overridable approval gate (.github/workflows/training-agent-storyboards.yml (modified) matches
.github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
…lidate_input required_tools (OR-gate) The per-storyboard gate (runner.js) admits a storyboard when any listed tool is present, while the suite selector (comply.js) requires all of them. With comply_test_controller in the list, every agent exposing the universal test controller was selected for this storyboard and then failed all 17 steps on the missing validate_input — on the hosted grader, 17 spurious failures on each of two registered agents (adcontextprotocol#7404). Same shape as adcontextprotocol#6774. Refs adcontextprotocol#7404 (bug 1 only).
… storyboard no longer selected With comply_test_controller off canonical_format_validate_input's required_tools, the /governance and /brand training tenants (which expose the controller but not validate_input) no longer select that storyboard, and the one step it used to contribute to their passing-step count goes away: governance 161 -> 160, brand 116 -> 115. Clean-storyboard floors are untouched (56/47 and 55/45 observed). Same tuples mirrored in run-storyboards-matrix.sh.
…ratchet guard run-storyboards-sharding pins the same (tenant, min_clean, min_passed) tuples a third time, so lowering a floor has to be stated there too — which is the point of that guard. Current surface only: governance 161 -> 160, brand 116 -> 115. The 3.0-compat block is untouched because canonical_format_validate_input is introduced_in 3.1 and is never selected in that bundle.
904aa37 to
7c2b8a1
Compare
There was a problem hiding this comment.
Ladon verdict: Escalate to human review
Escalate — gated path requires human/CODEOWNERS approval.
No blocking or medium findings: clean pass. The PR drops comply_test_controller from canonical_format_validate_input required_tools (OR-gate fix), carries a correctly-scoped patch changeset, has no dist mutation and no schema/wire drift; the floor de-ratchet is consistent across all pinning sites.
However, this PR modifies .github/workflows/training-agent-storyboards.yml, which matches the hard gated path .github/workflows/** (gated_paths: true), and review_decision is REVIEW_REQUIRED, not APPROVED. Per decision rule row 2, the outcome must be escalate until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
Escalation reasons
.github/workflows/training-agent-storyboards.yml(modified) is under the hard approval gate.github/workflows/**andreview_decisionisREVIEW_REQUIRED, notAPPROVED— human/CODEOWNERS approval required (row 2).
This is consistent with the prior decision (escalate for the same reason). The gate lifts the moment the review decision becomes APPROVED.
Why human review
- .github/workflows/training-agent-storyboards.yml (modified) matches gated path .github/workflows/** and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required (row 2)
- This PR touches a path under a hard, non-overridable approval gate (.github/workflows/training-agent-storyboards.yml (modified) matches
.github/workflows/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
Reviewed the exact five-file delta and the final green head. The selector correction is sound: validate_input is the sole capability gate, the one-step governance/brand de-ratchets match the now-skipped storyboard, all three ratchet copies stay aligned, and the 3.0 compatibility surface is untouched.
Summary
comply_test_controllerfromrequired_toolsoncanonical_format_validate_input. The per-storyboard gate (required_tools.some(...)) admits a storyboard when any listed tool is present, so any agent exposing the universal test controller was selected for this storyboard and then failed all 17 steps on the missingvalidate_input. On the hosted grader that is 17 spurious failures on each of two registered agents (Hosted grader: 11 failing steps remain on two cards after #7454/#7405/#7406 (sales 10, signals 1); budget-truncated runs still published as complete grades #7404, bug 1).validate_inputrun exactly as before; agents without it now get the coverage-gap skip the suite selector already gives them locally.Deliberate exclusions
Validation
Docker validation on refreshed head
7c2b8a1aa1a0374e4e82699bc0963ca30e94272b, after a freshnpm ci(Node 24.21.0; current-main SDK pin14.0.0-rc.35):node --test tests/run-storyboards-sharding.test.cjs tests/run-storyboards-schema-root-options.test.cjs tests/lint-compliance-source-authority.test.cjs tests/lint-compliance-packaged-refs.test.cjs tests/sdk-runner-capability-gates.test.cjs— 48/48 passed in 3.64s.npm run test:unit— 71 files / 1,063 tests passed in 12.19s.npm run typecheck— passed, including the test-aware fixed-trace rollout check.bash -n scripts/run-storyboards-matrix.sh, changeset scope, Changesets 3.0.2 status (patch), immutable artifact check, PR title check, andgit diff --check— passed.ADCP_SKIP_STORYBOARD_MATRIX=1; GitHub's full training-agent matrix is authoritative. Full exact-head CI passed 49/49; independent human/CODEOWNER approval remains required. No full local server-suite pass is claimed for this refresh.Backport to
3.1.xis #7406, bundled with the four #6774 storyboards that exist on that line.Refs #7404 (bug 1 only).
Floors (added after the first CI run)
Removing
comply_test_controllerfrom the gate means the/governanceand/brandtraining tenants — which expose the controller but notvalidate_input— stop selecting this storyboard, and each loses the single passing step it contributed. CI caught that against the ratchet, correctly:So this branch also lowers those two
min_passing_stepsby one, in all three places the tuples are pinned:.github/workflows/training-agent-storyboards.yml,scripts/run-storyboards-matrix.sh, and therun-storyboards-shardingratchet guard. Clean-storyboard floors are untouched, no other tenant moves, and the3.0-compatsurface is untouched because the storyboard isintroduced_in: "3.1"and is never selected in that bundle.Flagging it explicitly since a lowered ratchet is exactly what that guard exists to make someone look at: the step count drops because the storyboard is no longer graded against agents that cannot run it, which is the point of the fix.
Current-main refresh (2026-09-14)
Current head:
7c2b8a1aa1a0374e4e82699bc0963ca30e94272b, rebased from904aa37b8788e18f8481c2738ff7d016bac65839onto GitHub main8d50c0e3c303391c8bb35c5042248eb847b06bb9.git range-diff 4fceb127c..904aa37b 8d50c0e3..7c2b8a1amarks all three commits=. The #7480 overlap applied cleanly: its complete schema snapshot/cleanup logic remains intact; this PR changes only the governance and brand current-floor tuples in that script. The reviewed YAML correction, controller prerequisites, fixture/vector references, current artifact-action versions, and floor tests are preserved. Scope remains five files, +17/−7, without dist or workspace artifacts.#7453/#7454 landed the service fixes. This protocol correction still needs to merge and release; public hosted cards cannot be expected to clear from service changes alone. #7406 remains unchanged and waits for this main PR to merge. No merge is authorized for this refresh; #7404 remains open.
Conductor workspace · Session
Final exact-head gates
All gates passed on
7c2b8a1aa1a0374e4e82699bc0963ca30e94272b: 47 GitHub check runs, the additional GitGuardian check, and IPR status (49 passing entries ingh pr checks; zero pending or failed). These include Build Check, the complete Training Agent Storyboards matrix, CodeQL, changeset policy, and IPR. No CodeQL PR comments.The current tenant logs confirm both sides of the required-tools gate:
Fresh Ladon review found no blocking or medium issues on this exact head, but escalated to human/CODEOWNER approval because
.github/workflows/training-agent-storyboards.ymlis a gated path. This is a required human approval, not an automated approval. Independent review was re-requested fromadcontextprotocol/maintainersafter the automated gates passed;bokelleyalso remains requested. The PR remains unmerged and #7406 is unchanged.